@drmingdrmer reviewed 2 of 4 files at r1, all commit messages.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions

src/meta/api/src/data_mask_api_impl.rs line 105 at r1 (raw file):

                )
                .into());
            }

This function looks strange:

• fetch_id can be moved out from the transaction-retry-loop
• it does not need a loop to retry: just one transaction trial would be enough.

Code quote:

            let row_access_name_ident = RowAccessPolicyNameIdent::new(
                name_ident.tenant().clone(),
                name_ident.data_mask_name().to_string(),
            );
            if self.get_pb(&row_access_name_ident).await?.is_some() {
                return Err(AppError::DatamaskAlreadyExists(
                    name_ident.exist_error("name conflicts with an existing row access policy"),
                )
                .into());
            }

src/meta/api/src/data_mask_api_impl.rs line 112 at r1 (raw file):

            // data mask name -> data mask table id list

            let id = fetch_id(self, IdGenerator::data_mask_id()).await?;

fetch_id can be moved out from the transaction-retry-loop.

Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @drmingdrmer)

src/meta/api/src/data_mask_api_impl.rs line 105 at r1 (raw file):

Done.

src/meta/api/src/data_mask_api_impl.rs line 112 at r1 (raw file):

Done.

@drmingdrmer reviewed 2 of 3 files at r3, all commit messages.
Reviewable status: 4 of 5 files reviewed, 4 unresolved discussions

src/meta/api/src/row_access_policy_api_impl.rs line 132 at r3 (raw file):

            Ok(Ok(CreateRowAccessPolicyReply { id: row_access_id }))
        } else {
            Err(MetaTxnError::TxnRetryMaxTimes(TxnRetryMaxTimes::new(

same here

src/meta/api/src/data_mask_api_impl.rs line 149 at r3 (raw file):

            })
        } else {
            Err(KVAppError::from(TxnRetryMaxTimes::new(func_name!(), 1)))

if it fails. the else branch of the txn should try to get the existing record and here it should be a Existing error, not a max-retry txn error.

Reviewable status: 3 of 18 files reviewed, 4 unresolved discussions (waiting on @drmingdrmer)

src/meta/api/src/data_mask_api_impl.rs line 149 at r3 (raw file):

Done.

@drmingdrmer reviewed 13 of 15 files at r4, all commit messages.
Reviewable status: 16 of 18 files reviewed, 4 unresolved discussions (waiting on @TCeason)

src/meta/api/src/data_mask_api_impl.rs line 72 at r4 (raw file):

            return Ok(Err(
                name_ident.exist_error("name conflicts with an existing masking policy")
            ));

this check before the insert can be removed?

Code quote:

        let row_access_name_ident = RowAccessPolicyNameIdent::new(
            name_ident.tenant().clone(),
            name_ident.data_mask_name().to_string(),
        );
        if self.get_pb(&row_access_name_ident).await?.is_some() {
            return Ok(Err(
                name_ident.exist_error("name conflicts with an existing masking policy")
            ));

src/meta/api/src/row_access_policy_api_impl.rs line 72 at r4 (raw file):

                name_ident.exist_error("name conflicts with an existing masking policy")
            ));
        }

this check before the insert can be removed?

Code quote:

        let mask_name_ident = DataMaskNameIdent::new(
            name_ident.tenant().clone(),
            name_ident.row_access_name().to_string(),
        );
        if self.get_pb(&mask_name_ident).await?.is_some() {
            return Ok(Err(
                name_ident.exist_error("name conflicts with an existing masking policy")
            ));
        }

@drmingdrmer reviewed 1 of 15 files at r4, 2 of 2 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TCeason)